feat: Update gRPC definitions for Limit Enforcer#12
Open
Rerowros wants to merge 1 commit intoPasarGuard:mainfrom
Open
feat: Update gRPC definitions for Limit Enforcer#12Rerowros wants to merge 1 commit intoPasarGuard:mainfrom
Rerowros wants to merge 1 commit intoPasarGuard:mainfrom
Conversation
- Update service.proto with node_id, panel_api_url, limit params - Regenerate service_pb2.py and service_grpc.py - Update abstract_node.py with limit enforcer params - Update grpclib.py and rest.py with new start method signature
WalkthroughConfiguration parameters for limit enforcement (node_id, panel_api_url, limit_check_interval, limit_refresh_interval) are added to the PasarGuardNode.start method signature across the abstract interface and concrete implementations, with corresponding protobuf message schema updates to transmit these settings. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jan 17, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@PasarGuardNodeBridge/common/service.proto`:
- Around line 26-31: The proto comments saying "default 30/60" are misleading
because proto3 scalars decode to zero; update handling for the fields
limit_check_interval and limit_refresh_interval so zero values don't become
0-second intervals: either change the proto to use optional or wrapper types
(e.g., google.protobuf.Int32Value) for limit_check_interval and
limit_refresh_interval so presence can be detected, or keep scalars but add
normalization logic in the node/panel code that reads node_id, panel_api_url,
limit_check_interval, and limit_refresh_interval and replaces 0 with the
intended defaults (30 and 60) before using them.
🧹 Nitpick comments (2)
PasarGuardNodeBridge/rest.py (1)
115-149: Avoid mutable default forexclude_inbounds.Using a list literal as a default argument can leak state across calls. Prefer
Noneand initialize inside the method.♻️ Proposed fix
- async def start( + async def start( self, config: str, backend_type: service.BackendType, users: list[service.User], keep_alive: int = 0, - exclude_inbounds: list[str] = [], + exclude_inbounds: list[str] | None = None, timeout: int | None = None, # Limit enforcer configuration (optional - sent to node for real-time enforcement) node_id: int = 0, panel_api_url: str = "", limit_check_interval: int = 30, limit_refresh_interval: int = 60, ): """Start the node with proper task management""" timeout = timeout or self._default_timeout + exclude_inbounds = exclude_inbounds or []PasarGuardNodeBridge/grpclib.py (1)
100-130: Avoid mutable default forexclude_inbounds.Use
None+ initialization to avoid shared state across calls.♻️ Proposed fix
- async def start( + async def start( self, config: str, backend_type: service.BackendType, users: list[service.User], keep_alive: int = 0, - exclude_inbounds: list[str] = [], + exclude_inbounds: list[str] | None = None, timeout: int | None = None, # Limit enforcer configuration (optional - sent to node for real-time enforcement) node_id: int = 0, panel_api_url: str = "", limit_check_interval: int = 30, limit_refresh_interval: int = 60, ) -> service.BaseInfoResponse | None: """Start the node with proper task management""" timeout = timeout or self._default_timeout + exclude_inbounds = exclude_inbounds or []
Comment on lines
+26
to
+31
|
|
||
| // Limit enforcer configuration (sent from panel) | ||
| int32 node_id = 6; | ||
| string panel_api_url = 7; | ||
| int32 limit_check_interval = 8; // seconds, default 30 | ||
| int32 limit_refresh_interval = 9; // seconds, default 60 |
There was a problem hiding this comment.
Proto3 “default” comments are not enforced.
In proto3, scalar fields default to zero on decode; the “default 30/60” comments won’t apply unless the server/client normalizes. This can lead to 0-second intervals if another client omits the fields.
Consider either:
- Normalizing
0to30/60in the node/panel logic, or - Using
optional/wrapper types to detect presence.
💡 Minimal clarification (comment-only) if you keep current schema
- int32 limit_check_interval = 8; // seconds, default 30
- int32 limit_refresh_interval = 9; // seconds, default 60
+ int32 limit_check_interval = 8; // seconds; 0 means "use default" (e.g., 30)
+ int32 limit_refresh_interval = 9; // seconds; 0 means "use default" (e.g., 60)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Limit enforcer configuration (sent from panel) | |
| int32 node_id = 6; | |
| string panel_api_url = 7; | |
| int32 limit_check_interval = 8; // seconds, default 30 | |
| int32 limit_refresh_interval = 9; // seconds, default 60 | |
| // Limit enforcer configuration (sent from panel) | |
| int32 node_id = 6; | |
| string panel_api_url = 7; | |
| int32 limit_check_interval = 8; // seconds; 0 means "use default" (e.g., 30) | |
| int32 limit_refresh_interval = 9; // seconds; 0 means "use default" (e.g., 60) |
🤖 Prompt for AI Agents
In `@PasarGuardNodeBridge/common/service.proto` around lines 26 - 31, The proto
comments saying "default 30/60" are misleading because proto3 scalars decode to
zero; update handling for the fields limit_check_interval and
limit_refresh_interval so zero values don't become 0-second intervals: either
change the proto to use optional or wrapper types (e.g.,
google.protobuf.Int32Value) for limit_check_interval and limit_refresh_interval
so presence can be detected, or keep scalars but add normalization logic in the
node/panel code that reads node_id, panel_api_url, limit_check_interval, and
limit_refresh_interval and replaces 0 with the intended defaults (30 and 60)
before using them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates the gRPC service definitions to support per-node limit configuration.
Changes
service.protowith new fields for limit enforcement params.service_pb2.pyandservice_grpc.py.Related PRs
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.